Skip to content

262 Fix improper code dependency#264

Merged
m7pr merged 17 commits into
mainfrom
262_get_code@main
Aug 7, 2025
Merged

262 Fix improper code dependency#264
m7pr merged 17 commits into
mainfrom
262_get_code@main

Conversation

@m7pr

@m7pr m7pr commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #262 and insightsengineering/teal.gallery#218

Alternative solution: #263

Functions were kept on the left side of the dependency graph, which lead to cyclic unwanted dependencies.
Now all functions, even when used on the LHS are moved to the RHS of the dependency graph.

devtools::load_all(".")
devtools::load_all("../teal.data")
data <- teal_data()
data <- within(data, {
  library(random.cdisc.data)
  library(nestcolor)
  
  ADSL <- radsl(seed = 1)
  ADMH <- radmh(ADSL, seed = 1)
  ADVS <- radvs(ADSL, seed = 1)
  teal.data::col_labels(ADMH[c("MHDISTAT")]) <- c("Status of Disease")
  
  ADVS <- dplyr::inner_join(x = ADVS, y = ADSL[, c("STUDYID", "USUBJID"), drop = FALSE], by = c("STUDYID", "USUBJID"))
  
})
data@code
cat(get_code(data, names = "ADVS"))
library(random.cdisc.data)
library(nestcolor)
ADSL <- radsl(seed = 1)
ADVS <- radvs(ADSL, seed = 1)
ADVS <- dplyr::inner_join(x = ADVS, y = ADSL[, c("STUDYID", "USUBJID"), drop = FALSE], by = c("STUDYID", "USUBJID"))

@m7pr m7pr added the core label Aug 5, 2025
@m7pr m7pr mentioned this pull request Aug 5, 2025
Closed
@github-actions

github-actions Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------
R/qenv-c.R                          55       0  100.00%
R/qenv-class.R                      13       0  100.00%
R/qenv-concat.R                      7       0  100.00%
R/qenv-constructor.R                 1       0  100.00%
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  60       1  98.33%   40
R/qenv-extract.R                    30       0  100.00%
R/qenv-get_code.R                   24       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   27
R/qenv-get_messages.r                5       0  100.00%
R/qenv-get_outputs.R                 6       0  100.00%
R/qenv-get_var.R                    26       0  100.00%
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        7       7  0.00%    137-151
R/qenv-length.R                      2       1  50.00%   2
R/qenv-show.R                       29      29  0.00%    19-50
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      254       3  98.82%   160, 258, 331
R/utils.R                           42       0  100.00%
TOTAL                              581      46  92.08%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/utils-get_code_dependency.R       +7      +1  -0.37%
TOTAL                               +7      +1  -0.08%

Results for commit: 02b1b3b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions

github-actions Bot commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Unit Tests Summary

  1 files   14 suites   6s ⏱️
171 tests 168 ✅ 3 💤 0 ❌
261 runs  258 ✅ 3 💤 0 ❌

Results for commit 02b1b3b.

♻️ This comment has been updated with latest results.

@m7pr m7pr mentioned this pull request Aug 5, 2025
Closed
gogonzo
gogonzo reviewed Aug 6, 2025
View reviewed changes
Comment thread R/utils-get_code_dependency.R
gogonzo
gogonzo reviewed Aug 6, 2025
View reviewed changes

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test demonstrating recursive loop (reason for the PR)

@m7pr
m7pr requested a review from Copilot August 6, 2025 12:11
Copilot AI reviewed Aug 6, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes improper code dependency handling by preventing functions from being positioned on the left-hand side of dependency graphs, which was causing unwanted cyclic dependencies. The solution moves all functions to the right-hand side of the dependency graph, even when they appear on the LHS of assignments.

  • Introduces a new function move_functions_after_arrow to handle function positioning in dependency chains
  • Modifies the extract_occurrence function to apply the function movement logic

Comment thread R/utils-get_code_dependency.R
Comment thread R/utils-get_code_dependency.R
Comment thread R/utils-get_code_dependency.R
@github-actions

github-actions Bot commented Aug 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_get_code 👶 $+0.07$ doesn_t_consider_function_called_on_the_lhs_as_a_dependent_in_this_call_dependency_in_further_calls_

Results for commit f2a1b19

♻️ This comment has been updated with latest results.

@m7pr
m7pr requested a review from gogonzo August 6, 2025 12:55
gogonzo
gogonzo reviewed Aug 6, 2025
View reviewed changes
Comment thread tests/testthat/test-qenv_get_code.R Outdated
Comment thread R/utils-get_code_dependency.R Outdated
Comment thread R/utils-get_code_dependency.R Outdated
Comment thread R/utils-get_code_dependency.R Outdated
Comment thread R/utils-get_code_dependency.R Outdated
m7pr and others added 6 commits August 6, 2025 16:39
@m7pr @gogonzo
Update tests/testthat/test-qenv_get_code.R
5cf4a86 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr @gogonzo
Update R/utils-get_code_dependency.R
3f408c5 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr @gogonzo
Update R/utils-get_code_dependency.R
758917c 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr @gogonzo
Update R/utils-get_code_dependency.R
3c0c360 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr @gogonzo
Update R/utils-get_code_dependency.R
49d888d 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@github-actions
[skip style] [skip vbump] Restyle files
c29bb03
gogonzo
gogonzo approved these changes Aug 7, 2025
View reviewed changes

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge after fixing cicd

Comment thread R/utils-get_code_dependency.R Outdated
m7pr and others added 2 commits August 7, 2025 10:57
@m7pr @gogonzo
Update R/utils-get_code_dependency.R
c50f686 
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@m7pr
superlintr
06bb551
@m7pr
m7pr enabled auto-merge (squash) August 7, 2025 09:01
@m7pr
m7pr merged commit 3af8c95 into main Aug 7, 2025
26 checks passed
@m7pr
m7pr deleted the 262_get_code@main branch August 7, 2025 12:05
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments
+1 more reviewer
@gogonzo gogonzo gogonzo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Improper code dependency

3 participants

@m7pr @gogonzo